Skip to content

fix(review): bind exact reviews to the leased PR head - #981

Draft
vincentkoc wants to merge 3 commits into
mainfrom
fix/exact-review-materialization
Draft

fix(review): bind exact reviews to the leased PR head#981
vincentkoc wants to merge 3 commits into
mainfrom
fix/exact-review-materialization

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes an issue where exact pull request reviews could claim one immutable head SHA while reviewing the target branch checkout instead, and OpenClaw reviews could run without the required sibling Codex source.

Why This Change Was Made

Exact PR runs now delegate leased-head fetch, validation, and detached checkout to a testable TypeScript command. A moved head returns a structured source_drift result that skips review, artifact creation, and lease cleanup while the durable queue requeues the latest source. OpenClaw review workers also materialize openai/codex beside the target checkout.

User Impact

Maintainers can trust that an exact ClawSweeper verdict applies to the SHA named by its queue lease and that Codex-backed OpenClaw contract checks have the required dependency source. There is no new configuration.

Evidence

  • Exact head: 302b879791f77c94305190d1e8ce730421209497 (signed).
  • pnpm run build:repair passed.
  • node --test test/repair/exact-review-source.test.ts test/sweep-workflow.test.ts: 107/107 passed, including matching head, source drift, unfetchable ref, and a shallow divergent-history fixture with a 75-commit PR plus 60 newer base commits.
  • git diff --check: clean.
  • Fresh uncommitted and committed-branch autoreview: 0 accepted/actionable findings; committed review confidence 0.90.
  • Full local pnpm run check: build, format, lint, coverage thresholds, and 2,847 tests passed; five unchanged Linux Landlock fixture tests cannot import on macOS because Apple libc has no capset. Exact-head Linux CI is the authoritative remaining gate.

Real Behavior Proof

  • Claim: exact PR review cannot proceed on a checkout that differs from the leased source SHA.
  • Exercised surface: exact-review target materialization, structured source-drift output, downstream review/artifact/lease gates, and OpenClaw Codex materialization.
  • Scenario: real temporary bare Git remotes cover a matching PR head, a force-moved PR head, and a missing PR ref.
  • Observed result: matching heads detach-checkout the exact leased SHA; moved heads return source_drift without changing the target checkout; missing refs fail closed. A shallow target whose base advanced 60 commits and whose PR contains 75 commits is fully unshallowed, preserves the original merge base, and exposes all 75 PR commits to origin/main..HEAD. Parsed workflow coverage proves source drift skips reservation, review, ledger finalization, artifact bundling, and unsuccessful lease release while exporting requeue_latest=true.
  • Limits: production exact-review confirmation requires this draft to pass Linux CI and later be deployed.

ClawSweeper Finding Disposition - July 31, 2026

  • Move leased-head handling out of workflow shell: addressed by repair:exact-review-source; workflow YAML now only clones the target, invokes the command, and branches on structured outputs.
  • Replace command-text assertions: addressed with real Git behavior tests for matching, moved, and unfetchable heads; workflow tests retain only wiring and side-effect-fence assertions.
  • Prevent every review-result side effect on drift: explicit guards now cover reservation, review, ledger finalization, artifact creation, and lease cleanup. Command-status reporting intentionally remains enabled so maintainers see that the latest source is waiting for review.
  • Requeue newest source reliably: source drift completes the generation as success with requeue_latest=true; fetch failure remains a hard failure rather than a false requeue.
  • Preserve PR ancestry beyond the fixed fetch depth: addressed by removing the PR-side depth cap, passing the canonical base branch into the source guard, and calling the shared merge-base materializer after checkout. The new divergent-history fixture starts from a depth-1 base clone and proves both sides are complete before review.
  • Remove the release-owned changelog entry: not applied. Current origin/main AGENTS.md reserves the OpenClaw target repository's CHANGELOG.md during foreign PR work; CONTRIBUTING.md explicitly says ClawSweeper follows its own release-note policy. This PR changes ClawSweeper itself, and the operationally meaningful exact-review change therefore keeps its ClawSweeper changelog entry.
  • Exact-head CI is now complete: pnpm check, CodeQL, containment smoke, sparse repair build, Windows launcher, and production automerge E2E are green.

@vincentkoc
vincentkoc force-pushed the fix/exact-review-materialization branch from 85284ab to 12b5e4b Compare July 31, 2026 18:28
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 31, 2026
@clawsweeper

clawsweeper Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 3, 2026, 5:05 AM ET / 09:05 UTC.

ClawSweeper review

What this changes

This PR adds a tested TypeScript source guard that fetches and detached-checks out the leased pull-request head, skips review side effects when the source has moved, and materializes sibling Codex source for OpenClaw review jobs.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Keep this PR open for maintainer integration. The proposed guard makes the leased PR SHA authoritative before review work begins, and the supplied proof plus focused source inspection support the design; however, GitHub reports the branch as dirty against current main, whose workflow has since gained queue-admission and finalization safeguards. The author is a repository member, so this item is non-closeable by automation. Likely related people: Peter Steinberger (recent workflow owner, high confidence), Vincent Koc (recent exact-review contributor, high confidence), and Martin Cleary (exact-review lifecycle contributor, medium confidence).

Priority: P2
Reviewed head: 302b879791f77c94305190d1e8ce730421209497
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) This is a well-scoped, strongly evidenced automation fix with no actionable patch defect found; rebasing is still required to integrate it safely with newer workflow changes.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body provides after-fix terminal proof from real temporary Git remotes, showing matching heads detach-check out the lease, moved heads return a no-op source-drift result, unavailable refs fail closed, and shallow divergent history retains the merge base.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides after-fix terminal proof from real temporary Git remotes, showing matching heads detach-check out the lease, moved heads return a no-op source-drift result, unavailable refs fail closed, and shallow divergent history retains the merge base.
Evidence reviewed 5 items Leased source is verified before checkout: The proposed source guard fetches refs/pull/<number>/head, compares its full SHA with the leased SHA, returns source_drift without checkout on mismatch, and detached-checks out the leased SHA before merge-base preparation on a match.
Workflow fences downstream review effects on drift: The PR adds the guard before the exact review and conditions reservation, review, ledger finalization, artifact bundling, and unsuccessful lease release on checkout-target.outputs.source_drift != 'true'; the matching workflow test asserts these fences.
Real Git coverage targets the failure modes: The new focused test constructs temporary bare Git remotes and covers matching heads, force-moved heads, unavailable PR refs, and shallow divergent ancestry; the PR body also records successful focused and full checks.
Findings None None.
Security None None.

How this fits together

The exact-review workflow leases a specific GitHub item revision, prepares a target repository checkout, then runs Codex review and durable artifact/ledger publication. This change sits between checkout and review so every downstream result is tied to the leased pull-request head or safely requeued when that head moved.

flowchart LR
  Queue[Exact review queue] --> Lease[Leased PR head]
  Lease --> Checkout[Target repository checkout]
  Checkout --> Guard[Exact source guard]
  Guard -->|Leased head matches| Review[Codex review]
  Guard -->|Source drift| Requeue[Requeue latest source]
  Review --> Finalize[Artifact and ledger finalization]
Loading

Decision needed

Question Recommendation
Should this maintainer-authored exact-review safety fix be rebased and integrated with current workflow safeguards before landing? Rebase and retain both protections: Resolve the workflow against current main, keeping the exact leased-head guard and all later queue, acknowledgement, publication, and finalization safety changes.

Why: The intended guard is sound, but the dirty merge requires choosing and validating the combined workflow semantics; automated cleanup must not resolve a repository-member PR or decide which newer lifecycle protections remain authoritative.

Before merge

  • Resolve merge risk (P1) - GitHub reports this draft as dirty against current main; an incorrect conflict resolution could omit newer queue-admission, command-acknowledgement, publication, or finalization safeguards from the merged workflow.
  • Resolve merge risk (P1) - The patch changes exact-review queue semantics, so its focused real-Git proof should be rerun after the rebased workflow resolution rather than relying only on the existing head’s evidence.
  • Complete next step (P2) - This repository-member PR needs maintainer-controlled conflict resolution and a fresh exact-head proof/review; there is no narrow independent repair to dispatch.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Change surface 7 files affected; 432 added, 14 removed The patch spans the exact-review workflow, a new repair command, and behavior-focused regression coverage.
Focused regression coverage 160 new test lines plus 87 workflow-test additions The new real-Git fixture covers source match, source drift, missing ref, and divergent shallow-history behavior.

Merge-risk options

Maintainer options:

  1. Rebase with lifecycle safeguards intact (recommended)
    Resolve the dirty workflow merge by retaining current main’s queue admission, acknowledgement, publication, and finalization paths alongside the exact-source guard, then rerun focused proof.
  2. Pause for a narrower replacement
    If the combined workflow cannot be proved safely after rebase, pause this branch and submit a narrow replacement against current main.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Rebase onto current main, preserve queue admission, acknowledgement, publication, and finalization safeguards together with the exact-source guard, then rerun focused exact-review source and workflow validation.

Technical review

Best possible solution:

Rebase the PR onto current main, resolve .github/workflows/sweep.yml by preserving both the leased-head source guard and all newer exact-review lifecycle safeguards, then rerun the focused source-guard/workflow checks and obtain a fresh review for that exact head.

Do we have a high-confidence way to reproduce the issue?

Yes. The proposed real-Git fixture supplies a high-confidence path for a lease/head mismatch: move refs/pull/<number>/head after the lease, then verify the guard returns source_drift before any review-result side effect.

Is this the best way to solve the issue?

Yes, conditionally. Moving leased-head materialization into a focused TypeScript command is a more testable boundary than workflow shell logic, but it must be rebased so the final workflow retains all current exact-review safeguards.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 2d1a40bfa643.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: This is a bounded review-automation correctness fix with important but indirect maintainer-facing impact.
  • merge-risk: 🚨 automation: The patch changes the workflow that leases, checks out, reviews, requeues, and finalizes exact-review jobs.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides after-fix terminal proof from real temporary Git remotes, showing matching heads detach-check out the lease, moved heads return a no-op source-drift result, unavailable refs fail closed, and shallow divergent history retains the merge base.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal proof from real temporary Git remotes, showing matching heads detach-check out the lease, moved heads return a no-op source-drift result, unavailable refs fail closed, and shallow divergent history retains the merge base.

Evidence

What I checked:

  • Leased source is verified before checkout: The proposed source guard fetches refs/pull/<number>/head, compares its full SHA with the leased SHA, returns source_drift without checkout on mismatch, and detached-checks out the leased SHA before merge-base preparation on a match. (src/repair/exact-review-source.ts:31, 302b879791f7)
  • Workflow fences downstream review effects on drift: The PR adds the guard before the exact review and conditions reservation, review, ledger finalization, artifact bundling, and unsuccessful lease release on checkout-target.outputs.source_drift != 'true'; the matching workflow test asserts these fences. (.github/workflows/sweep.yml:892, 302b879791f7)
  • Real Git coverage targets the failure modes: The new focused test constructs temporary bare Git remotes and covers matching heads, force-moved heads, unavailable PR refs, and shallow divergent ancestry; the PR body also records successful focused and full checks. (test/repair/exact-review-source.test.ts:1, 302b879791f7)
  • Current main requires a careful workflow integration: The PR merge base is 3637a9424ec20396a3903cd79cffb2e4e360bc28; current main includes later workflow changes for throttled live-item handling, terminal acknowledgement, publication, and closure finalization, while GitHub reports this PR as dirty. (.github/workflows/sweep.yml:780, a769f25c3500)
  • Feature-history routing: Current-main workflow history after the PR base includes recent exact-review safety work by Peter Steinberger, while Vincent Koc's merged commit 0925ecae37878788fd25977e5a96b8bdaf4bc363 updated exact-review publication state and Martin Cleary introduced the lifecycle projection. (.github/workflows/sweep.yml:451, 0925ecae3787)

Likely related people:

  • Peter Steinberger: Current main contains several post-base exact-review workflow safety commits by Peter, including the latest workflow commit affecting finalization behavior. (role: recent workflow contributor; confidence: high; commits: a769f25c3500, 7e4d8cb7cee4, f43618fd2cae; files: .github/workflows/sweep.yml, test/sweep-workflow.test.ts)
  • Vincent Koc: Vincent authored this PR and also has merged current-main history on exact-review publication state, connecting the proposed source guard to an established review-lifecycle area. (role: recent exact-review contributor; confidence: high; commits: 0925ecae3787, 302b879791f7; files: .github/workflows/sweep.yml, src/repair/exact-review-source.ts)
  • Martin Cleary: Martin's merged lifecycle projection is a central antecedent of the workflow state this PR extends. (role: exact-review lifecycle contributor; confidence: medium; commits: b07ab751813c; files: .github/workflows/sweep.yml, test/sweep-workflow.test.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Rebase onto current main while retaining the newer exact-review lifecycle safeguards.
  • Rerun the focused exact-review source and workflow checks on the rebased head, then request a fresh review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (19 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-02T08:55:06.575Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T12:47:35.836Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T15:26:11.506Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T17:35:29.303Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T21:05:11.759Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T01:08:08.294Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T02:51:50.000Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T06:40:30.548Z sha 302b879 :: needs maintainer review before merge. :: none

@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 31, 2026
@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot removed the proof: sufficient Contributor real behavior proof is sufficient. label Jul 31, 2026
@vincentkoc vincentkoc mentioned this pull request Jul 31, 2026
Draft
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 1, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 3, 2026
@vincentkoc
vincentkoc force-pushed the fix/exact-review-materialization branch from 302b879 to 813167f Compare August 4, 2026 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vincentkoc